Stop using error_chain! in examples #102
Conversation
There was a problem hiding this comment.
Personally, I prefer expect/unwrap over Box<dyn std::error::Error> in tests because of the verbose error formatting. It's a matter of taste so I won't block the PR on this
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
faern
force-pushed
the
remove-error-chain-from-examples
branch
from
f5f8fe4 to
f5c3042
Compare
There was a problem hiding this comment.
Do you mean like this? I don't regard examples as "tests". They are supposed to showcase how to use the library. And this way it's littered with unidiomatic error handling. But yes, I agree that it both produce better debug info on failure, and it contains no boilerplate just for doing sane errors in the examples, so it's probably the least bad solution 👍
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @Serock3)
There was a problem hiding this comment.
Sure, an argument could be made for showcasing idiomatic error handling in examples, but in my opinion ? doesn't really accomplish that on its own, and it would only be necessary if the error handling is non-trivial.
It's a shame that the expects often break the line and make it slightly more verbose. I think being even more unidiomatic withunwraps instead could be worth it for readability, but I'll leave it up to you to decide.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)
examples/flush_rules.rs line 16 at r2 (raw file):
for anchor_name in env::args().skip(1) { match pf.flush_rules(&anchor_name, pfctl::RulesetKind::Filter) {
What is the purpose of this match statement? Is it not equivalent to
pf.flush_rules(&anchor_name, pfctl::RulesetKind::Filter).expect("Unable to flush filter rules");
println!("Flushed filter rules under anchor {}", anchor_name)
There was a problem hiding this comment.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @Serock3)
examples/flush_rules.rs line 16 at r2 (raw file):
Previously, Serock3 (Sebastian Holmin) wrote…
What is the purpose of this match statement? Is it not equivalent to
pf.flush_rules(&anchor_name, pfctl::RulesetKind::Filter).expect("Unable to flush filter rules"); println!("Flushed filter rules under anchor {}", anchor_name)
Yeah, that makes sense. I can't remember exactly why this was written this way. Fixed.
faern
force-pushed
the
remove-error-chain-from-examples
branch
from
273ffb8 to
16449a3
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
We want to get rid of
error-chainas a dependency completely. Some work towards this has been started in #88. However, for now that PR keepserror-chainfor the examples. I approach the problem from the other direction with this PR: Getting rid oferror-chainin the examples first. They are simpler and require less nice API surface since they are just example binaries, not a public library API.Here I opted for simply going with
Result<..., Box<dyn std::error::Error>>. This has the positive side of being very simple. But with the downside that we don't have an error chain in the same way. We lose some of the context we previously had with an error. This might make it slightly harder to track down what failed when examples fail. On the other hand, examples should focus on showcasing how to use the main crate, not how to do ideal error handling. I'm open to discuss alternative solutions. We could replace allchain_errwithexpectand have the examples panic instead. In practice it won't make much of a difference since that's basically what returning an error from main does anyway 🤷. However, that's arguably even more "ugly" error handling thanBox<dyn Error>.This change is